feat(crypto): CRP-2597 Extend NiDkgTag with HighThresholdForKey variant #2445
Conversation
fspreiss
changed the base branch from
franzstefan/CRP-2597-move-masterpublickeyid-proto-to-types-ontopof-non-copy-ni-dkg-id
to
franzstefan/CRP-2597-move-masterpublickeyid-proto-to-types
…r_public_key_when_parsing_proto
fspreiss
changed the title
| NiDkgTag::HighThresholdForKey(master_public_key_id) => { | ||
| error!( | ||
| log, | ||
| "Implementation error: NiDkgTag::HighThresholdForKey({master_public_key_id}) used in SetupInitialDKG for callback ID {callback_id}", | ||
| ); | ||
| } |
There was a problem hiding this comment.
I think the idea here is to reuse the transcripts_for_remote_subnets field also for xnet reshares of VetKD keys. So then we would generalize this function such that it can generate the correct ConsensusResponse, depending on whether the callback ID is for a SetupInitialDKG context or a VetKD reshare context.
For now, logging an error and linking the ticket to generalize this function (CON-1416) should be fine.
| //////////////////////////////////////////////////////////////////////////////// | ||
| // TODO: how to behave here? The code below is copied+adapted from the HighThreshold case. However, | ||
| // this code currently won't be executed because we iterate over TAGS, which is a const that does not | ||
| // and cannot contain an NiDkgTag::HighThresholdForKey entry | ||
| /////////////////////////////////////////////////////////////////////////////// | ||
| NiDkgTag::HighThresholdForKey(master_public_key_id) => { | ||
| let resharing_transcript = reshared_transcripts | ||
| .get(&NiDkgTag::HighThresholdForKey(master_public_key_id.clone())); | ||
| ( | ||
| resharing_transcript | ||
| .map(|transcript| transcript.committee.get().clone()) | ||
| .unwrap_or_else(|| node_ids.clone()), | ||
| resharing_transcript.cloned(), | ||
| ) | ||
| } |
There was a problem hiding this comment.
I think the behavior is correct (maybe it could even be combined with the NiDkgTag::HighThreshold branch). We can link ticket CON-1413 above, reminding us to iterate over all vetKD keys that were requested by registry in addition to the tags in TAGS.
| ///////////////////////////////////////// | ||
| // TODO: check with Consensus team | ||
| NiDkgTag::HighThresholdForKey(_) => panic!("not applicable"), | ||
| ///////////////////////////////////////// |
There was a problem hiding this comment.
I think we can link CON-1417 to extend this test once we have support for vet key transcripts in registry CUPs
| ///////////////////////////////////////// | ||
| // TODO: check with Consensus team | ||
| NiDkgTag::HighThresholdForKey(_) => panic!("not applicable"), | ||
| ///////////////////////////////////////// | ||
| } |
There was a problem hiding this comment.
We can link CON-1413 to extend this test once we can create local transcript configs for vetKeys that were requested by registry
| ) | ||
| })?; | ||
| //////////////////////////////////////////////////////// | ||
| // TODO: Consensus team to decide if we should rather |
There was a problem hiding this comment.
I think we can leave it as is for now, but in the future we should probably just replace pb::TaggedNiDkgTranscript with pb::NiDkgTranscript since storing the tag (and key Id) twice is redundant. But this would require working around the cup_compatibilty_test some more, so I think we can handle this in a separate consensus ticket.
| )) | ||
| })?, | ||
| ///////////////////////////////////////////////// | ||
| // TODO: ideally we extend existing tests where build_tagged_transcripts_map |
There was a problem hiding this comment.
It could be that there are no specific unit tests for the (de)serialization of general summaries, and that this is only tested by system/integration tests. For IDKG we have test_summary_proto_conversion, probably it makes sense to add something similar for vet KD once that is possible.
It will also be tested as part of the cup_compatibility_test but only once we include the new variant in the ExhaustiveSet.
Extends the
NiDkgTagwith a new variantNiDkgTag::HighThresholdForKeythat holds aMasterPublicKeyId.Notes:
Makes the representation for
NiDkgTagexplicit with#[repr(isize)]because a#[repr(inttype)]must be provided on an enum if it has a non-unit variant with a discriminant (E0732). The representation should be the same as it was before, given thatisizeis the default representation for enums.Note that casting
NiDkgTagwithas, e.g., toi32or evenisizeis no longer possible because "anasexpression can be used to convert enum types to numeric types only if the enum type is unit-only or field-less" (see Enumeration casting and E0605).The conversion
impl TryFrom<i32> for NiDkgTaghad to be removed, because it is no longer possible, because ani32itself is no longer sufficient for instantiating anNiDkgTag.This conversion was used in two places: (1)
impl TryFrom<NiDkgIdProto> for NiDkgIdand (2)build_tagged_transcripts_map. There, the conversion is now implemented directly.No longer Implements
EnumIterforNiDkgTagbecause this would require aDefaultimplementation forMasterPublicKeyId, which does not make sense, at least in production. Instead implementsEnumCount.TODOs:
NiDkgTag::HighThresholdForKeyvsNiDkgTag::HighThresholdForKeyId#[allow(clippy::result_large_err)]in crypto code